Return generic error messages to clients for server-side errors by ChristianPavilonis · Pull Request #530 · IABTechLab/trusted-server

ChristianPavilonis · 2026-03-19T19:56:34Z

Summary

Prevent server-side error details (store names, config paths, proxy addresses) from leaking to clients by replacing the user_message() passthrough with categorized responses
Client errors (4xx) retain brief, safe descriptions; server/integration errors (5xx/502/503) return a generic "An internal error occurred"
Add unit tests asserting the security boundary: all server-error variants must return only the generic message

Changes

File	Change
`crates/trusted-server-core/src/error.rs`	Replace `self.to_string()` in `user_message()` with a `match` that surfaces safe messages for client errors and a generic message for server errors. Add doc comments on `BadRequest` (user-visible message warning), `GdprConsent` (why detail is suppressed), and `user_message` trait method (updated semantics). Add two unit tests covering all variants.

Closes

Closes #437

Test plan

cargo test --workspace
cargo clippy --all-targets --all-features -- -D warnings
cargo fmt --all -- --check
JS tests: cd crates/js/lib && npx vitest run
JS format: cd crates/js/lib && npm run format
Docs format: cd docs && npm run format
WASM build: cargo build --bin trusted-server-fastly --release --target wasm32-wasip1
Manual testing via fastly compute serve

Checklist

Changes follow CLAUDE.md conventions
No unwrap() in production code — use expect("should ...")
Uses log macros (not println!)
New code has tests
No secrets or credentials committed

prk-Jr

Summary

This hardens client-facing error bodies and the overall direction looks right. GitHub CI is passing, but I found a couple of follow-up improvements around classification drift and regression coverage.

Non-blocking

🤔 thinking

InvalidUtf8 is currently surfaced as Invalid request data, but its only producer today is embedded config decoding in settings_data.rs, so it still reads like a client error for a server bootstrap failure.

♻️ refactor

status_code() and user_message() now encode the exposure policy in separate matches. Centralizing that mapping behind a small helper or enum would make it harder for classification and response text to drift apart.

crates/trusted-server-core/src/error.rs

aram356

@ChristianPavilonis Please resolve conflicts

Replace user_message() passthrough of Display output with a match that returns generic messages for 5xx/502/503 errors while keeping brief descriptions for 4xx client errors. Full error details are already logged via log::error! and never lost. Closes #437

aram356

Summary

Well-scoped PR that prevents server-side error details from leaking to clients. The secure-by-default wildcard pattern (new variants automatically get the generic message) is the right approach for a security boundary.

Blocking

❓ question

InvalidUtf8 classification: Only produced by settings_data.rs:24 (server bootstrap), yet classified as a 4xx client error returning "Invalid request data". Should this fall through to the generic message? (crates/trusted-server-core/src/error.rs:131)
GdprConsent message suppression: The only 4xx error that fully hides its detail message. The rationale (consent strings may contain user data) is sound, but confirming intent and adding a doc comment on the variant would help future maintainers. (crates/trusted-server-core/src/error.rs:127)

Non-blocking

🤔 thinking

status_code() and user_message() can drift apart: Both methods encode client-vs-server classification in separate match blocks. If a new variant is classified as 4xx in status_code() but hits the wildcard in user_message(), it would return 400 with "An internal error occurred" — confusing but safe by default. Not a concern for this PR, but worth noting.

⛏ nitpick

BadRequest doc comment precision: Says "message is returned to clients" but the message is formatted via format!("Bad request: {message}"), not returned verbatim. (crates/trusted-server-core/src/error.rs:20)

🏕 camp site

PR body references old crate path crates/common/src/error.rs — the crate was renamed to trusted-server-core in PR #517.
PR checklist says "Uses tracing macros" but per CLAUDE.md the project uses the log crate.

CI Status

fmt: PASS
clippy: PASS
rust tests: PASS
js tests: PASS
format checks: PASS

crates/trusted-server-core/src/error.rs

…dd doc comments

aram356

Summary

Security-focused change that replaces the user_message() passthrough (which exposed internal error details like store names, config paths, proxy addresses) with categorized responses: client errors (4xx) get brief safe descriptions, server errors (5xx/502/503) get a generic message. Well-scoped single-file change with good test coverage.

Non-blocking

🤔 thinking

Wildcard match may silently suppress future client errors: The _ => catch-all in user_message() defaults new variants to the generic message, even if they're 4xx client errors. Consider matching exhaustively to force explicit decisions. (error.rs:134)

♻️ refactor

BadRequest format string duplicated: user_message() and #[display] both hardcode "Bad request: {message}" — could diverge silently. (error.rs:128)

🌱 seedling

Consider a compile-time guarantee that status_code() and user_message() stay in sync: Both methods independently classify variants as client vs. server errors. A future follow-up could extract the classification into a single method (e.g., is_client_error()) or match exhaustively in both to prevent divergence.

📝 note

InvalidUtf8 status code change is correct: The 400 → 500 change is appropriate — the only usage is for the embedded TOML settings file, which is a server-side resource.

CI Status

fmt: PASS
clippy: PASS
rust tests: PASS
js tests: PASS
integration tests: PASS
browser integration tests: PASS
CodeQL: PASS

aram356 · 2026-03-24T20:44:18Z

crates/trusted-server-core/src/error.rs

+            Self::InvalidHeaderValue { .. } => "Invalid header value".to_string(),
+            // Server/integration errors (5xx/502/503) — generic message only.
+            // Full details are already logged via log::error! in to_error_response.
+            _ => "An internal error occurred".to_string(),


🤔 thinking — Wildcard match may silently suppress future client errors

The _ => catch-all means any new variant added to TrustedServerError will default to the generic "An internal error occurred" message — even if it's a 4xx client error. This is safe-by-default (no leaks), but it could silently give users unhelpful messages for legitimate client errors.

Consider matching exhaustively (no _) so the compiler forces an explicit decision when new variants are added. The status_code() method already matches exhaustively, so user_message() would stay consistent.

Alternatively, the existing test server_errors_return_generic_message partially mitigates this — but only if someone remembers to add new variants to the test.

aram356 · 2026-03-24T20:44:18Z

crates/trusted-server-core/src/error.rs

-        self.to_string()
+        match self {
+            // Client errors (4xx) — safe to surface a brief description
+            Self::BadRequest { message } => format!("Bad request: {message}"),


♻️ refactor — BadRequest format string duplicated between Display and user_message()

user_message() returns format!("Bad request: {message}") which duplicates the #[display("Bad request: {message}")] attribute. If one changes, the other could silently diverge.

Suggestion: Reuse Display for the BadRequest arm:

Self::BadRequest { .. } => self.to_string(),

aram356 · 2026-03-24T20:44:18Z

crates/trusted-server-core/src/error.rs

            Self::InsecureDefault { .. } => StatusCode::INTERNAL_SERVER_ERROR,
            Self::InvalidHeaderValue { .. } => StatusCode::BAD_REQUEST,
-            Self::InvalidUtf8 { .. } => StatusCode::BAD_REQUEST,
+            Self::InvalidUtf8 { .. } => StatusCode::INTERNAL_SERVER_ERROR,


📝 note — InvalidUtf8 status code change from 400 → 500 is correct

The only usage is for the embedded trusted-server.toml file (settings_data.rs:24), which is a server-side resource. Invalid UTF-8 there is a server error, not a client error.

aram356

Summary

Clean, focused security fix that correctly addresses #437. The user_message() method previously exposed internal details (store names, IP addresses, config paths) to clients. Now server/integration errors (5xx/502/503) return a generic string while client errors (4xx) retain safe descriptions.

Non-blocking

🌱 seedling

status_code / user_message consistency test: The wildcard _ arm in user_message() catches everything with the generic message. If someone adds a new 4xx variant to status_code but forgets to update user_message, it would silently get the 500-style generic message. A test that asserts all 4xx variants have a specific (non-generic) user_message would catch this.

📝 note

Tests use .into() for String construction: CLAUDE.md prefers Type::from(value) over .into() for clarity. Common Rust idiom in tests though — not worth blocking on.

👍 praise

Correct reclassification of InvalidUtf8 from 400 to 500
Thoughtful suppression of GdprConsent details (consent strings may contain user data)
Comprehensive test coverage of all error variants

CI Status

All 13 checks: PASS

aram356 · 2026-03-30T06:54:20Z

crates/trusted-server-core/src/error.rs

    fn user_message(&self) -> String {
-        // Use the Display implementation which already has the specific error message
-        self.to_string()
+        match self {


🤔 thinking — BadRequest passes caller-provided strings directly to clients

The BadRequest variant echoes its message field verbatim to clients. While the doc comment warns callers to keep it free of internal details, there's no compile-time or runtime enforcement. Current call sites are safe, but as more are added a caller could accidentally pass sensitive details. A future refinement (e.g., a newtype ClientMessage(String) or a fixed enum of safe messages) could prevent this.

aram356 · 2026-03-30T06:54:20Z

crates/trusted-server-core/src/error.rs

+    /// return a generic message. Full error details are preserved in server logs.
    fn user_message(&self) -> String;
 }



👍 praise — Correct reclassification of InvalidUtf8 to 500

The only usage is in settings_data.rs for the embedded TOML config — a server-side data integrity issue, not client input. Changing from BAD_REQUEST to INTERNAL_SERVER_ERROR is the right call.

aram356 · 2026-03-30T06:54:20Z

crates/trusted-server-core/src/error.rs

@@ -30,6 +34,10 @@ pub enum TrustedServerError {
    #[display("GAM error: {message}")]


👍 praise — Thoughtful handling of GdprConsent

Suppressing detail for consent errors (consent strings may contain user data) while still returning the category name is a well-reasoned middle ground. Good doc comment explaining the rationale.

aram356 · 2026-03-30T06:54:20Z

crates/trusted-server-core/src/error.rs

-        self.to_string()
+        match self {
+            // Client errors (4xx) — safe to surface a brief description
+            Self::BadRequest { message } => format!("Bad request: {message}"),


🏕 camp site — InvalidHeaderValue message could include the header name

Since this is a 400 (client error), including which header was invalid would help debugging. Current usage in cookies.rs has "Cookie header contains invalid UTF-8" in the message field, but that detail is now suppressed by the match. Safe and correct as-is — just a campsite opportunity.

ChristianPavilonis force-pushed the fix/generic-error-messages branch from 58c5906 to 820599c Compare March 19, 2026 21:03

ChristianPavilonis requested review from aram356 and prk-Jr and removed request for aram356 March 19, 2026 21:22

ChristianPavilonis marked this pull request as ready for review March 19, 2026 21:24

aram356 assigned ChristianPavilonis Mar 20, 2026

prk-Jr reviewed Mar 20, 2026

View reviewed changes

crates/trusted-server-core/src/error.rs Outdated Show resolved Hide resolved

crates/trusted-server-core/src/error.rs Show resolved Hide resolved

aram356 requested changes Mar 20, 2026

View reviewed changes

ChristianPavilonis force-pushed the fix/generic-error-messages branch from 0e8dc86 to b955ece Compare March 20, 2026 19:17

ChristianPavilonis requested a review from aram356 March 20, 2026 19:17

Merge branch 'main' into fix/generic-error-messages

5936a4e

aram356 reviewed Mar 20, 2026

View reviewed changes

crates/trusted-server-core/src/error.rs Show resolved Hide resolved

crates/trusted-server-core/src/error.rs Outdated Show resolved Hide resolved

crates/trusted-server-core/src/error.rs Outdated Show resolved Hide resolved

Address PR review feedback: reclassify InvalidUtf8 as server error, a…

763ee4a

…dd doc comments

ChristianPavilonis requested a review from aram356 March 23, 2026 15:04

Merge branch 'main' into fix/generic-error-messages

2d1e798

aram356 reviewed Mar 24, 2026

View reviewed changes

Merge branch 'main' into fix/generic-error-messages

3bc2956

aram356 approved these changes Mar 30, 2026

View reviewed changes

		@@ -30,6 +34,10 @@ pub enum TrustedServerError {
		#[display("GAM error: {message}")]

Conversation

ChristianPavilonis commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Closes

Test plan

Checklist

Uh oh!

prk-Jr left a comment

Choose a reason for hiding this comment

Summary

Non-blocking

🤔 thinking

♻️ refactor

Uh oh!

Uh oh!

Uh oh!

aram356 left a comment

Choose a reason for hiding this comment

Uh oh!

aram356 left a comment

Choose a reason for hiding this comment

Summary

Blocking

❓ question

Non-blocking

🤔 thinking

⛏ nitpick

🏕 camp site

CI Status

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aram356 left a comment

Choose a reason for hiding this comment

Summary

Non-blocking

🤔 thinking

♻️ refactor

🌱 seedling

📝 note

CI Status

Uh oh!

aram356 Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

aram356 Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

aram356 Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

aram356 left a comment

Choose a reason for hiding this comment

Summary

Non-blocking

🌱 seedling

📝 note

👍 praise

CI Status

Uh oh!

aram356 Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

aram356 Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

aram356 Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

aram356 Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

ChristianPavilonis commented Mar 19, 2026 •

edited

Loading